Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix ensure_monomorphic_enough #136839

Merged
merged 2 commits into from
Feb 11, 2025

Conversation

lukas-code
Copy link
Member

@lukas-code lukas-code commented Feb 10, 2025

When polymorphization was still a thing, the visitor was used to only recurse into used generic parameters of function/closure/coroutine types and allow unused parameters (i.e. the polymorphized parameters) to remain generic.

When polymorphization got removed, this got changed to always treat all parameters as polymorphic and never recurse into them: /~https://github.com/rust-lang/rust/pull/133883/files#diff-210c59e321070d0ca4625c04e9fb064bf43ddc34082e7e33a7ee8a6c577e95afL44-L62

This is clearly wrong and can cause MIR opts to misbehave, for example this currently prints "false" in release mode:

#![feature(core_intrinsics)]

fn generic<T>() {}

const fn type_id_of_val<T: 'static>(_: &T) -> u128 {
    std::intrinsics::type_id::<T>()
}

fn cursed_is_i32<T: 'static>() -> bool {
    (const { type_id_of_val(&generic::<T>) } == const { type_id_of_val(&generic::<i32>) })
}

fn main() {
    dbg!(cursed_is_i32::<i32>());
}

This PR reverts to the old behavior of always treating all types that contain type parameters as too generic, like we used to do without -Zpolymorphize before.

I'm not including the above as a test case here, because I think there is little value in testing code paths that have been removed and this seems unlikely to regress in a way that would be caught by a regression test, but let me know if you disagree and want me to add a test anyway.

@rustbot
Copy link
Collaborator

rustbot commented Feb 10, 2025

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 10, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 10, 2025

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

@lukas-code
Copy link
Member Author

r? @saethlin

@rustbot rustbot assigned saethlin and unassigned petrochenkov Feb 10, 2025
@saethlin
Copy link
Member

This is obviously wrong

I don't think that's a fair characterization, considering that what is apparently a soundness hole made it through code review and almost to stable.

I think we have plenty of regression tests already which the same characterization would apply to. I would rather have the test.

@lukas-code lukas-code force-pushed the actually-monomorphic-enough branch from 63ecba7 to c1da4f1 Compare February 11, 2025 00:18
@lukas-code
Copy link
Member Author

This is obviously wrong

I don't think that's a fair characterization, considering that what is apparently a soundness hole made it through code review and almost to stable.

Fair. I'm not trying to throw shade here.

I think we have plenty of regression tests already which the same characterization would apply to. I would rather have the test.

Sure, added a test.

@compiler-errors
Copy link
Member

compiler-errors commented Feb 11, 2025

I do think it's obviously wrong, but also you may want to choose your words more carefully next time 😃 Things that seem obviously wrong in hindsight are often overlooked. In this case, I probably just mis-reasoned when reviewing about the polarity of the .is_used that was being removed; that should've been folded into a true rather than a false. Mistakes are made all the time. 🤷

@bors r+ rollup

This is a simple backport, should be relatively uncontroversial but also should be done before stable artifacts are prepared soon for 1.85.

@rustbot label: beta-nominated

@rustbot rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 11, 2025
@bors
Copy link
Contributor

bors commented Feb 11, 2025

📌 Commit c1da4f1 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 11, 2025
jhpratt added a commit to jhpratt/rust that referenced this pull request Feb 11, 2025
…ough, r=compiler-errors

fix ensure_monomorphic_enough

When polymorphization was still a thing, the visitor was used to only recurse into *used generic parameters* of function/closure/coroutine types and allow unused parameters (i.e. the polymorphized parameters) to remain generic.

When polymorphization got removed, this got changed to always treat all parameters as polymorphic and never recurse into them: /~https://github.com/rust-lang/rust/pull/133883/files#diff-210c59e321070d0ca4625c04e9fb064bf43ddc34082e7e33a7ee8a6c577e95afL44-L62

This is clearly wrong and can cause MIR opts to misbehave, for example this currently prints "false" in release mode:

```rust
#![feature(core_intrinsics)]

fn generic<T>() {}

const fn type_id_of_val<T: 'static>(_: &T) -> u128 {
    std::intrinsics::type_id::<T>()
}

fn cursed_is_i32<T: 'static>() -> bool {
    (const { type_id_of_val(&generic::<T>) } == const { type_id_of_val(&generic::<i32>) })
}

fn main() {
    dbg!(cursed_is_i32::<i32>());
}
```

This PR reverts to the old behavior of always treating all types that contain type parameters as too generic, like we used to do without `-Zpolymorphize` before.

~~I'm not including the above as a test case here, because I think there is little value in testing code paths that have been removed and this seems unlikely to regress in a way that would be caught by a regression test, but let me know if you disagree and want me to add a test anyway.~~
@RalfJung
Copy link
Member

I don't think that's a fair characterization, considering that what is apparently a soundness hole made it through code review and almost to stable.

Which soundness hole are you referring to here?

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2025
Rollup of 11 pull requests

Successful merges:

 - rust-lang#136606 (Fix long lines which rustfmt fails to format)
 - rust-lang#136663 (Stabilize `NonZero::count_ones`)
 - rust-lang#136672 (library: doc: core::alloc::Allocator: trivial typo fix)
 - rust-lang#136704 (Improve examples for file locking)
 - rust-lang#136721 (cg_llvm: Reduce visibility of some items outside the `llvm` module)
 - rust-lang#136813 (rustc_target: Add the fp16 target feature for AArch32)
 - rust-lang#136830 (fix i686-unknown-hurd-gnu x87 footnote)
 - rust-lang#136832 (Fix platform support table for i686-unknown-uefi)
 - rust-lang#136835 (Stop using span hack for contracts feature gating)
 - rust-lang#136837 (Overhaul how contracts are lowered on fn-like bodies)
 - rust-lang#136839 (fix ensure_monomorphic_enough)

r? `@ghost`
`@rustbot` modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2025
Rollup merge of rust-lang#136839 - lukas-code:actually-monomorphic-enough, r=compiler-errors

fix ensure_monomorphic_enough

When polymorphization was still a thing, the visitor was used to only recurse into *used generic parameters* of function/closure/coroutine types and allow unused parameters (i.e. the polymorphized parameters) to remain generic.

When polymorphization got removed, this got changed to always treat all parameters as polymorphic and never recurse into them: /~https://github.com/rust-lang/rust/pull/133883/files#diff-210c59e321070d0ca4625c04e9fb064bf43ddc34082e7e33a7ee8a6c577e95afL44-L62

This is clearly wrong and can cause MIR opts to misbehave, for example this currently prints "false" in release mode:

```rust
#![feature(core_intrinsics)]

fn generic<T>() {}

const fn type_id_of_val<T: 'static>(_: &T) -> u128 {
    std::intrinsics::type_id::<T>()
}

fn cursed_is_i32<T: 'static>() -> bool {
    (const { type_id_of_val(&generic::<T>) } == const { type_id_of_val(&generic::<i32>) })
}

fn main() {
    dbg!(cursed_is_i32::<i32>());
}
```

This PR reverts to the old behavior of always treating all types that contain type parameters as too generic, like we used to do without `-Zpolymorphize` before.

~~I'm not including the above as a test case here, because I think there is little value in testing code paths that have been removed and this seems unlikely to regress in a way that would be caught by a regression test, but let me know if you disagree and want me to add a test anyway.~~
@bors bors merged commit 2130310 into rust-lang:master Feb 11, 2025
12 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Feb 11, 2025
@lukas-code lukas-code deleted the actually-monomorphic-enough branch February 11, 2025 13:06
@apiraino
Copy link
Contributor

Beta backport approved as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle. Backport labels handled by them.

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Feb 13, 2025
@cuviper cuviper mentioned this pull request Feb 13, 2025
@cuviper cuviper modified the milestones: 1.86.0, 1.85.0 Feb 13, 2025
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 13, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 13, 2025
[beta] backports

- Pattern Migration 2024: try to suggest eliding redundant binding modifiers rust-lang#136577, rust-lang#136857
- chore: update rustc-hash 2.1.0 to 2.1.1 rust-lang#136605
- Make `AsyncFnOnce`, `AsyncFnMut`, `AsyncFn` non-`#[fundamental]` rust-lang#136724
- fix ensure_monomorphic_enough rust-lang#136839
- Revert "Stabilize `extended_varargs_abi_support`" rust-lang#136897, rust-lang#136934

r? cuviper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants